[Merged by Bors] - fetch: smarter peer selection #6477
Conversation
Add a possibility to select best peers by their supported protocols, which is used by syncv2. When doing hash requests, only query peers that support hs/1 and as/1 protocols. This avoids querying peers that didn't finish their initialization and thus didn't start their fetch servers yet.
ivan4th
requested review from
dshulyak,
fasmat,
poszu,
acud and
jellonek
as code owners
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## develop #6477 +/- ##
=======================================
Coverage 79.9% 79.9%
=======================================
Files 352 352
Lines 46410 46438 +28
=======================================
+ Hits 37094 37124 +30
+ Misses 7210 7209 -1
+ Partials 2106 2105 -1 ☔ View full report in Codecov by Sentry. |
| // SelectBestWithProtocols is similar to SelectBest but filters peers by supported protocols. | ||
| // If protocols is empty, it returns the best peers regardless of the protocol. | ||
| // If protocols is not empty, it returns the best peers that support at least one of the protocols. | ||
| func (p *Peers) SelectBestWithProtocols(n int, protocols []protocol.ID) []peer.ID { |
There was a problem hiding this comment.
I'm not fully understanding this - why only at least one protocol needs to be supported? The only place outside of tests where this is used activeSetProtocol and hashProtocol are requested, aren't both needed?
There was a problem hiding this comment.
The idea is that there may be multiple versions of syncv2 protocol deployed at some point in time: sync/2, sync/2.1 and so on. When choosing peers for multipeer sync, any of such nodes will do, it's just that different protocols will be in use, later versions being more traffic-efficient etc.
For the purpose of hash fetching, the requirement may be just one of activeSetProtocol / hashProtocol or all of them, depending on which IDs are being fetched. It maybe could make some sense to invent a more complex logic for paritioning the set of hashes to download by the protocol etc., but in practice the servers for activeSetProtocol and hashProtocol are enabled almost at the same instant at the end of the node initialization, and in a very unlikely case of a race the worst thing that may happen is current go-spacemesh behavior, that is, failed fetch which will be retried later, with failed peer getting lower priority when doing peer selection. So, in case of hash fetching, from practical standpoint it doesn't really matter whether we pass activeSetProtocol, hashProtocol or both to this function.
Co-authored-by: Matthias Fasching <[email protected]>
Co-authored-by: Matthias Fasching <[email protected]>
|
bors merge |
## Motivation Most of the time, there are some nodes that already joined the P2P network but didn't complete their initialization yet, and thus attempts to e.g. fetch blobs to them end up in stream setup errors (protocol not supported). Additionally, during initial phases of `syncv2` testing, we're going to have a limited number of nodes supporting `sync/2` protocol, and when choosing peers for `syncv2`, we must only include these peers. Co-authored-by: Ivan Shvedunov <[email protected]>
|
Build failed: |
|
Trying again |
|
bors merge |
## Motivation Most of the time, there are some nodes that already joined the P2P network but didn't complete their initialization yet, and thus attempts to e.g. fetch blobs to them end up in stream setup errors (protocol not supported). Additionally, during initial phases of `syncv2` testing, we're going to have a limited number of nodes supporting `sync/2` protocol, and when choosing peers for `syncv2`, we must only include these peers. Co-authored-by: Ivan Shvedunov <[email protected]>
|
Pull request successfully merged into develop. Build succeeded: |
spacemesh-bors
bot
changed the title
Motivation
Most of the time, there are some nodes that already joined the P2P
network but didn't complete their initialization yet, and thus
attempts to e.g. fetch blobs to them end up in stream setup errors
(protocol not supported).
Additionally, during initial phases of
syncv2testing, we're goingto have a limited number of nodes supporting
sync/2protocol, andwhen choosing peers for
syncv2, we must only include these peers.Description
Add a possibility to select best peers by their supported protocols,
which is used by
syncv2.When doing hash requests, only query peers that support hs/1 and as/1
protocols. This avoids querying peers that didn't finish their
initialization and thus didn't start their fetch servers yet.
Test Plan
Verify how sync works on a node